Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change EventList from Table subclass to attribute #754

Merged
merged 8 commits into from Nov 18, 2016

Conversation

cdeil
Copy link
Contributor

@cdeil cdeil commented Nov 4, 2016

This pull request:

  • Changes EventList from an astropy.table.Table sub-class to a class with an astropy.table.Table attribute.
  • Add EventList.select_row_subset method that dispatches to EventList.table and is called everywhere row subset selection occurs.
  • Changes GTI from an astropy.table.Table sub-class to a class with an astropy.table.Table attribute.
  • Add EventList test with a Fermi-LAT event list (make some IACT-specific columns optional, e.g. in print output)
  • Some fixes and improvement in EventListChecker
  • Adapts code using EventList
  • Misc cleanup and improvements of EventList related code

@cdeil cdeil added this to the 0.5 milestone Nov 4, 2016
@cdeil cdeil self-assigned this Nov 4, 2016
@joleroi
Copy link
Contributor

joleroi commented Nov 8, 2016

Please add a test for stacking event lists

@cdeil
Copy link
Contributor Author

cdeil commented Nov 10, 2016

@adonath @joleroi I'm not done here adapting the code and test to the new EventList class, and the diff is already messy and hard to review.

But maybe you can have a quick look at this
https://github.com/cdeil/gammapy/blob/ccc016af65defcd3b1143b9826895a1effbbb112/gammapy/data/event_list.py#L73
it's basically this now:

class EventList(object):
    def __init__(self, table):
        self.table = table

Any comments?

@adonath
Copy link
Member

adonath commented Nov 14, 2016

@cdeil I've had a quick look at the changes and I don't have any specific comments. I think introducing the .table attribute is a good choice, because it cleans up the EventList API and removes inheritance complications with methods such as .read(), .write() and .info().

table = Table.read(str(filename), **kwargs)
return cls(table=table)

def info(self, file=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change this to __str__ (and add an info method calling it if you like). see http://docs.gammapy.org/en/latest/development/howto.html#object-summary-info-string

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

@joleroi joleroi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cdeil Looks good from a very superficial look. Only one comment.

@cdeil cdeil changed the title Improve EventList class and tests Change EventList and GTI from Table subclass to attribute Nov 18, 2016
@cdeil cdeil mentioned this pull request Nov 18, 2016
4 tasks
@cdeil
Copy link
Contributor Author

cdeil commented Nov 18, 2016

I've finished up this PR and updated the description above.
I'd like to make the 0.5 release today, so some tasks were moved to a future reminder issue: #782

@cdeil
Copy link
Contributor Author

cdeil commented Nov 18, 2016

The only fail is this:
https://travis-ci.org/gammapy/gammapy/jobs/176966197#L2449
I'll merge this now, and update the notebook next.

@cdeil cdeil merged commit c3f14f0 into gammapy:master Nov 18, 2016
@cdeil cdeil changed the title Change EventList and GTI from Table subclass to attribute Change EventList from Table subclass to attribute Nov 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants